Avoiding direct `BuildConfig` usage #6517

ouchadam · 2022-07-11T11:45:08Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Moves the AnalyticsConfig to the Config
Introduces config based LocationConfig, VoipConfig, CryptoConfig
Reintroduces the live location sharing feature flag so that clients can opt out at runtime
Moves debug and application id to the injectable BuildMeta

Motivation and context

Avoids interacting directly with the BuildConfig outside the application level dagger module, this allows us to extract logic to modules which are relying on the build configuration

Screenshots / GIFs

No UI changes

Tests

General app smoke test

Tested devices

Physical
Emulator
OS version(s): 29

ouchadam · 2022-07-11T11:45:31Z

vector/build.gradle

@@ -284,7 +264,6 @@ android {
            versionName "${versionMajor}.${versionMinor}.${versionPatch}${getGplayVersionSuffix()}"

            resValue "bool", "isGplay", "true"
-            buildConfigField "boolean", "ALLOW_FCM_USE", "true"


ALLOW_FCM_USE is not used

ouchadam · 2022-07-11T11:46:27Z

vector/src/main/java/im/vector/app/core/di/ConfigurationModule.kt

+
+@InstallIn(SingletonComponent::class)
+@Module
+object ConfigurationModule {


a dedicated module to provide the abstractions for Config/BuildConfig

ouchadam · 2022-07-11T11:49:41Z

vector/src/main/java/im/vector/app/features/notifications/NotificationBroadcastReceiver.kt

@@ -48,31 +48,32 @@ class NotificationBroadcastReceiver : BroadcastReceiver() {
    @Inject lateinit var activeSessionHolder: ActiveSessionHolder
    @Inject lateinit var analyticsTracker: AnalyticsTracker
    @Inject lateinit var clock: Clock
+    @Inject lateinit var actionIds: NotificationActionIds


the notification ids include the application package name which is now injected

bmarty

This is a nice improvement. My 2 cents

bmarty · 2022-07-11T14:01:38Z

vector/src/main/java/im/vector/app/core/di/SingletonModule.kt

-    fun providesBuildMeta() = BuildMeta()
+    fun providesBuildMeta() = BuildMeta(
+            isDebug = BuildConfig.DEBUG,
+            sdkInt = Build.VERSION.SDK_INT,


sdkInt is a bit out of scope for BuildMeta object (I just see that BuildMeta is not new, was not aware of this class before), since Build.VERSION.SDK_INT have different values at runtime (and not at compile time). We have BuildVersionSdkIntProvider on the SDK, and also AndroidVersionTestOverrider (that looks a bit hacky, I would be happier to have a similar BuildVersionSdkIntProvider) app side, which allow to do some test and overriding.

very true, happy to swap out for the BuildVersionSdkIntProvider or a :vector version of the same class

used the existing provider 3bfe25a

bmarty · 2022-07-11T14:06:07Z

vector/src/main/java/im/vector/app/core/resources/BuildMeta.kt

 data class BuildMeta(
-        val sdkInt: Int = Build.VERSION.SDK_INT
+        val isDebug: Boolean,


Do we really need to propagate isDebug to submodules? I think if we chose the Debug (resp Release) buildType, all the submodules will use the same buildType, and so all BuildConfig.DEBUG will have the same values for all modules.

We could use the BuildConfig like you've mentioned, I would lean towards abstracting as it provides an entry point for testing, avoid modules needing to generate a BuildConfig and will still work if we start to use pure kotlin modules.

From a build/architecture perspective, ideally variants would only exist at the Android application module level rather than leaking the concept of an element application into the library modules, this will mainly benefit compilation time of multiple variants as the modules will only need to be compiled once for all variants

Good points. Thanks!

bmarty · 2022-07-11T14:07:04Z

vector/src/main/java/im/vector/app/features/home/HomeActivityViewModel.kt

@@ -81,7 +81,8 @@ class HomeActivityViewModel @AssistedInject constructor(
        private val analyticsStore: AnalyticsStore,
        private val lightweightSettingsStorage: LightweightSettingsStorage,
        private val vectorPreferences: VectorPreferences,
-        private val analyticsTracker: AnalyticsTracker
+        private val analyticsTracker: AnalyticsTracker,
+        private val analyticsConfig: AnalyticsConfig,


analyticsConfig appear to be unused, at least there is no other change form this file in GH interface.

it's because the previous import has been replaced with an instance property of the same name import im.vector.app.config.analyticsConfig -> private val analyticsConfig: AnalyticsConfig

https://github.com/vector-im/element-android/pull/6517/files#diff-7153f5b6cf4c510293d53c73474e854a3a84e6003c88341472c0808377bf3462R118

bmarty · 2022-07-11T14:09:11Z

vector/src/main/java/im/vector/app/features/notifications/NotificationActionIds.kt

+    val DISMISS_ROOM_NOTIF_ACTION = "${buildMeta.applicationId}.NotificationActions.DISMISS_ROOM_NOTIF_ACTION"
+    val TAP_TO_VIEW_ACTION = "${buildMeta.applicationId}.NotificationActions.TAP_TO_VIEW_ACTION"
+    val DIAGNOSTIC_ACTION = "${buildMeta.applicationId}.NotificationActions.DIAGNOSTIC"
+    val PUSH_ACTION = "${buildMeta.applicationId}.PUSH"


Since these are not const val anymore, I think the naming convention is not followed anymore here.

bmarty · 2022-07-11T14:11:05Z

vector/src/main/java/im/vector/app/features/notifications/NotificationUtils.kt

-/**
- * Util class for creating notifications.
- * Note: Cannot inject ColorProvider in the constructor, because it requires an Activity
- */


Always a bit sad to see comment blocks vanish.

I'm happy to add back, although I'm not sure this comment block adds much?

Note: Cannot inject ColorProvider in the constructor, because it requires an Activity

From what I can tell, this technically this isn't true, ColorProvider has an application context injected (unless I've missed something 🤔 )

Util class for creating notifications.

Could be avoided by renaming the file to NotificationFactory or NotificationCreator, which helps give the class clarity in its purpose (utility classes can be see as an anti pattern, although extensions get a free pass... 🙃 )

OK, this is old comment I think, you are right. Also I do not understand why this class has @Singleton. And yes this would definitely needs a renaming. This class is maybe 6 or 7 years old :).

bmarty · 2022-07-11T14:12:50Z

vector/src/main/java/im/vector/app/core/di/ConfigurationModule.kt

+    )
+
+    @Provides
+    fun providesVoipConfig() = VoipConfig(


💯 for splitting between several small config classes!

bmarty · 2022-07-12T11:18:27Z

More remarks:

There is some cleanup to do at https://github.com/vector-im/element-android/blob/dd397b9a48d8a00d042050ad7f0b59122e4a31b5/vector/src/main/java/im/vector/app/features/attachments/AttachmentsHelper.kt#L42
This is using BuildConfig from the SDK, this is not correct. We should use BuildMeta.lowPrivacyLoggingEnabled now. (the log could be moved to the implementation in TimelineFragment)
There are still some occurences of BuildConfig.DEBUG in the SDK, is the plan to replace all with BuildMeta?
Same remark for Build.VERSION.SDK_INT, we should use BuildVersionSdkIntProvider everywhere. Could be in a dedicated PR though.

ouchadam · 2022-07-12T12:22:44Z

More remarks:

There is some cleanup to do at https://github.com/vector-im/element-android/blob/dd397b9a48d8a00d042050ad7f0b59122e4a31b5/vector/src/main/java/im/vector/app/features/attachments/AttachmentsHelper.kt#L42

This is using BuildConfig from the SDK, this is not correct. We should use BuildMeta.lowPrivacyLoggingEnabled now. (the log could be moved to the implementation in TimelineFragment)

There are still some occurences of BuildConfig.DEBUG in the SDK, is the plan to replace all with BuildMeta?

Same remark for Build.VERSION.SDK_INT, we should use BuildVersionSdkIntProvider everywhere. Could be in a dedicated PR though.

ahh I had only checked the vector module BuildConfig

for the SDK config it depends if we want the values to be available for consumers of the library vs only us/source compilers, at the moment the artifact we release is always DEBUG=false https://github.com/vanniktech/gradle-maven-publish-plugin/blob/master/plugin/src/main/kotlin/com/vanniktech/maven/publish/MavenPublishPlugin.kt#L78

if we want to respect the debug config of the consumer app then the debug flag should be passed as part of the MatrixConfiguration, I would be tempted to make it public static for simplicity 🤔

to help keep this PR smaller I would prefer to make the changes within the SDK as part of a separate PR but will fix the other points you've mentioned 👍

bmarty

thanks for the update. one final remark

bmarty · 2022-07-13T09:17:29Z

vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt

            is EventSharedAction.OnUrlClicked -> {
                onUrlClicked(action.url, action.title)
            }
+


Strange to see all those new lines. Is there a reason?

will revert, it looks like the autoformatting is doing something strange

I have the same issue when formatting this file on other branches

… opt out rather than relying on the package name to not be vector

… debug toggle

…lds upon the application id

…build meta can't be injected before super application.onCreate

…ia the BuildMeta

…instead of the sdk flag

sonarcloud · 2022-08-02T17:05:27Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
3 Security Hotspots
0 Code Smells

35.5% Coverage
0.3% Duplication

ouchadam added the Z-Modularization label Jul 11, 2022

ouchadam commented Jul 11, 2022

View reviewed changes

ouchadam force-pushed the feature/adm/build-config branch from e37cdb2 to ce7b3f9 Compare July 11, 2022 11:55

bmarty reviewed Jul 11, 2022

View reviewed changes

ouchadam force-pushed the feature/adm/build-config branch from c16512d to 987e03a Compare July 12, 2022 12:52

bmarty approved these changes Jul 13, 2022

View reviewed changes

ouchadam force-pushed the feature/adm/build-config branch 2 times, most recently from 4c0dd91 to 7ed39ed Compare July 29, 2022 13:21

ouchadam added 18 commits August 2, 2022 14:31

moving analytics config to the config module, it's now up to forks to…

274b1a1

… opt out rather than relying on the package name to not be vector

moving the voice message limit to the config module

87a8c78

porting the onboarding variant selection to the config

e795e9d

ports the key sharing strategy to the config

f14aabc

lifting the live location sharing build flag to the config along with…

012485b

… debug toggle

creating a location sharing config for holding the map tiler key

56afac2

lifting voip setting to a voip config

b6d052e

replacing DEBUG build config flag with injectable property

cc4aae0

extracting the config models to their own files

9fe3fc6

accessing the notification action ids via a dedicated class which bui…

811b2e8

…lds upon the application id

lifting the low privacy build config to the config

d42c27b

moving git versioning to the buildMeta abstraction

df02f50

removing unused buildconfig option

18943e3

moving build flavor information to the build meta

cceac66

adding changelog entry

c07296c

removing unused imports and fixing missing full stop on docs

3d207f9

using Config strict mode flag directly within the Application as the …

fe1019f

…build meta can't be injected before super application.onCreate

extracting the notification action ids to their own file

cb59d82

ouchadam added 4 commits August 2, 2022 14:32

making use of the existing sdk int provider instead of supplying it v…

8b9d1bb

…ia the BuildMeta

updating variable casing as they're no longer const val or file level

9b1def3

using low privacy log from the vector module for code within vector, …

3c1e183

…instead of the sdk flag

removing unused import

d1a63cc

ouchadam force-pushed the feature/adm/build-config branch from 7ed39ed to d1a63cc Compare August 2, 2022 13:32

fixing import ordering

2ee52f2

ouchadam merged commit 8aaf185 into develop Aug 2, 2022

ouchadam deleted the feature/adm/build-config branch August 2, 2022 17:07

Florian14 mentioned this pull request Aug 26, 2022

Move BuildConfig fields to vector-configs module tchapgouv/tchap-android#717

Closed

giomfo mentioned this pull request Feb 3, 2023

Move BuildConfig fields to vector-configs module tchapgouv/tchap-android#830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoiding direct `BuildConfig` usage #6517

Avoiding direct `BuildConfig` usage #6517

ouchadam commented Jul 11, 2022

ouchadam Jul 11, 2022

ouchadam Jul 11, 2022

ouchadam Jul 11, 2022

bmarty left a comment

bmarty Jul 11, 2022

ouchadam Jul 11, 2022

ouchadam Jul 11, 2022

bmarty Jul 11, 2022

ouchadam Jul 11, 2022

bmarty Jul 11, 2022

bmarty Jul 11, 2022

ouchadam Jul 11, 2022

bmarty Jul 11, 2022

bmarty Jul 11, 2022

ouchadam Jul 11, 2022

bmarty Jul 11, 2022

ouchadam Jul 11, 2022 •

edited

Loading

bmarty Jul 11, 2022

bmarty Jul 11, 2022

bmarty commented Jul 12, 2022

ouchadam commented Jul 12, 2022 •

edited

Loading

bmarty left a comment

bmarty Jul 13, 2022

ouchadam Jul 13, 2022

sonarcloud bot commented Aug 2, 2022

Avoiding direct BuildConfig usage #6517

Avoiding direct BuildConfig usage #6517

Conversation

ouchadam commented Jul 11, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Jul 12, 2022

ouchadam commented Jul 12, 2022 • edited Loading

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 2, 2022

Avoiding direct `BuildConfig` usage #6517

Avoiding direct `BuildConfig` usage #6517

ouchadam Jul 11, 2022 •

edited

Loading

ouchadam commented Jul 12, 2022 •

edited

Loading